-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[#65618] Remove jQuery from core #19429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
28d3b33 to
dabe404
Compare
8ecb545 to
c7be23c
Compare
dabe404 to
12c2e01
Compare
8c63a12 to
5d26905
Compare
af9f3f5 to
84518cd
Compare
c7be23c to
c0ad5e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes jQuery dependencies from the OpenProject core codebase, replacing jQuery APIs with native DOM APIs and modern JavaScript/TypeScript alternatives. The changes include migrating event listeners, DOM manipulation, element selection, CSS manipulation, and AJAX operations away from jQuery while maintaining the same functionality.
Key changes:
- Replace jQuery selectors with
document.querySelector/querySelectorAll - Replace jQuery event binding (
.on(),.off()) with nativeaddEventListener/removeEventListener - Replace jQuery DOM manipulation methods with native alternatives
- Update type definitions from
JQuery.TriggeredEventto nativeEvent,MouseEvent,KeyboardEvent
Reviewed Changes
Copilot reviewed 138 out of 139 changed files in this pull request and generated 25 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/package.json | Adds dependencies for jQuery-free event delegation (@knowledgecode/delegate) and slide animations (es6-slide-up-down) |
| frontend/src/app/shared/helpers/dom-helpers.ts | New utility module providing jQuery-like helper functions using native DOM APIs |
| modules/*/frontend/module/git-actions-menu/git-actions-menu.directive.ts | Updates event type from JQuery.TriggeredEvent to Event |
| modules/budgets/frontend/module/augment/planned-costs-form.ts | Replaces jQuery event delegation with @knowledgecode/delegate library |
| frontend/src/turbo/turbo-global-listeners.ts | Replaces jQuery .each() with native forEach() |
| frontend/src/app/shared/helpers/link-handling/link-handling.ts | Updates event type from JQuery.TriggeredEvent to MouseEvent |
| frontend/src/app/shared/helpers/drag-and-drop/dom-autoscroll.service.ts | Replaces jQuery event binding with native addEventListener |
| frontend/src/app/shared/directives/*/*.ts | Multiple directive files updating from jQuery element references to native HTMLElement |
| frontend/src/app/shared/components/*/*.ts | Component files migrating from jQuery to native DOM APIs for element manipulation and event handling |
| frontend/src/app/features/work-packages/components/wp-table/*.ts | Work package table components replacing jQuery with native DOM operations |
| frontend/src/app/features/calendar/*.ts | Calendar components removing jQuery tooltip and DOM manipulation calls |
| frontend/src/app/core/setup/globals/*.ts | Global listener setup replacing jQuery with native event handling |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
frontend/src/app/shared/helpers/drag-and-drop/dom-autoscroll.service.ts
Outdated
Show resolved
Hide resolved
frontend/src/app/shared/helpers/drag-and-drop/dom-autoscroll.service.ts
Outdated
Show resolved
Hide resolved
frontend/src/app/core/setup/globals/global-listeners/settings.ts
Outdated
Show resolved
Hide resolved
frontend/src/app/core/setup/globals/global-listeners/danger-zone-validation.ts
Outdated
Show resolved
Hide resolved
frontend/src/app/core/setup/globals/global-listeners/setup-server-response.ts
Outdated
Show resolved
Hide resolved
frontend/src/app/core/setup/globals/global-listeners/setup-server-response.ts
Outdated
Show resolved
Hide resolved
...p/features/work-packages/components/wp-fast-table/builders/relations/relation-row-builder.ts
Show resolved
Hide resolved
frontend/src/app/features/work-packages/components/wp-fast-table/handlers/row/click-handler.ts
Outdated
Show resolved
Hide resolved
c0ad5e2 to
07e86f2
Compare
frontend/src/app/core/setup/globals/global-listeners/setup-server-response.ts
Fixed
Show fixed
Hide fixed
...tend/src/app/shared/components/op-context-menu/handlers/op-context-menu-trigger.directive.ts
Fixed
Show fixed
Hide fixed
frontend/src/app/shared/components/op-context-menu/op-context-menu.service.ts
Fixed
Show fixed
Hide fixed
0b5ff43 to
24f8775
Compare
frontend/src/app/features/boards/board/add-card-dropdown/add-card-dropdown-menu.directive.ts
Fixed
Show fixed
Hide fixed
...tend/src/app/shared/components/op-context-menu/handlers/op-columns-context-menu.directive.ts
Fixed
Show fixed
Hide fixed
...tend/src/app/shared/components/op-context-menu/handlers/op-context-menu-trigger.directive.ts
Fixed
Show fixed
Hide fixed
...nd/src/app/shared/components/op-context-menu/handlers/op-settings-dropdown-menu.directive.ts
Fixed
Show fixed
Hide fixed
...tend/src/app/shared/components/op-context-menu/handlers/wp-create-settings-menu.directive.ts
Fixed
Show fixed
Hide fixed
...ponents/op-context-menu/icon-triggered-context-menu/icon-triggered-context-menu.component.ts
Fixed
Show fixed
Hide fixed
frontend/src/app/shared/components/op-context-menu/wp-context-menu/wp-single-context-menu.ts
Fixed
Show fixed
Hide fixed
.../src/app/shared/components/op-context-menu/wp-context-menu/wp-view-context-menu.directive.ts
Fixed
Show fixed
Hide fixed
24f8775 to
9e4b33c
Compare
frontend/src/app/shared/components/op-context-menu/op-context-menu-handler.ts
Fixed
Show fixed
Hide fixed
| dropDown.querySelector<HTMLAnchorElement>('li > a:first-child')?.focus(); | ||
| // when clicking on something, which is not the menu, close the menu | ||
| jQuery('html').on('click', { menu: menu.get(0) }, closeMenu); | ||
| document.addEventListener('click', (evt) => closeMenu(menu, evt), { once: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Human:
@copilot check this code has been ported correctly.
| // TODO: port tooltips | ||
| // jQuery(event.el).tooltip({ | ||
| // content: this.tooltipContentString(event.event.extendedProps.entry as TimeEntryResource, schema), | ||
| // items: '.fc-event', | ||
| // close() { | ||
| // document.querySelectorAll('.ui-helper-hidden-accessible').forEach(element => element.remove()); | ||
| // }, | ||
| // track: true, | ||
| // }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| fadeout.css('background', `-webkit-linear-gradient(${hslaStart} 0%, ${hslaEnd} 100%`); | ||
| fadeout.style.background = `-webkit-linear-gradient(${hslaStart} 0%, ${hslaEnd} 100%`; | ||
|
|
||
| ['-moz-linear-gradient', '-o-linear-gradient', 'linear-gradient', '-ms-linear-gradient'].forEach((style) => { | ||
| fadeout.css('background-image', `${style}(${hslaStart} 0%, ${hslaEnd} 100%`); | ||
| fadeout.style.backgroundImage = `${style}(${hslaStart} 0%, ${hslaEnd} 100%`; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot These vendor prefixes should no longer be necessary. Please create a PR to remove them.
| const td = this.findCell(fieldName); | ||
| td.addClass(editModeClassName); | ||
| let width = parseInt(td.css('width')); | ||
| const td = this.findCell(fieldName)!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| jRow.find(`.${relationCellClassName}`).empty(); | ||
| jRow.find(`.${relationCellClassName}.${columnId}`).append(relationLabel); | ||
| row.querySelector(`.${relationCellClassName}`)!.innerHTML = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // B: calculate the minimal width based on the window width | ||
| const width = this.$element.children().width()!; // A | ||
| // const width = jQuery('body').width(); // B | ||
| const width = this.element.children[0]?.clientWidth || document.body.clientWidth; // A with fallback to B |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| refreshView(vp:TimelineViewParameters) { | ||
| this.innerHeader = this.$element.find('.wp-table-timeline--header-inner'); | ||
| this.innerHeader = this.element.querySelector('.wp-table-timeline--header-inner')!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frontend/src/app/shared/components/fields/display/display-field-renderer.ts
Outdated
Show resolved
Hide resolved
| requestAnimationFrame(() => { | ||
| if (!this.active || this.openSeq !== seq) { | ||
| this.isOpening = false; | ||
| return; | ||
| } | ||
|
|
||
| void this.reposition(event) | ||
| .then(() => { | ||
| if (this.active && this.openSeq === seq) { | ||
| hostEl.style.visibility = 'visible'; | ||
| requestAnimationFrame(() => { | ||
| // Defer onOpen to next frame to ensure styles are applied | ||
| if (this.active && this.openSeq === seq) { | ||
| this.active.onOpen(this.activeMenu); | ||
| } | ||
| }); | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| // Fail-safe: close if positioning fails | ||
| console.error('Context menu positioning failed:', err); | ||
| if (this.openSeq === seq) this.close(); | ||
| }) | ||
| .finally(() => { | ||
| if (this.openSeq === seq) this.isOpening = false; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reposition (which now returns a Promise) and then schedule focussing in the next frame. I've added in guards against possible race conditions, but can't help feel this is a tad defensive.
See separate PR #20923 where I worked on this, rubber ducking with Copilot.
| this.$targetNotification.prop('hidden', true); | ||
| }); | ||
| slideUp(targetNotification, 400); | ||
| setTimeout(() => { targetNotification.hidden = true; }, 400); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally slideUp would set hidden for us.
| y: evt.clientY - mousedownY, | ||
| }); | ||
| element.dispatchEvent( | ||
| new CustomEvent('op:dragscroll', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
op:dragscroll anywhere?
Co-authored-by: myabc <[email protected]>
Co-authored-by: myabc <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, what a PR 🤯 🌟 Well done 👍 👏
I have two inline remarks where I was not sure, about the implications. However, honestly spoken, the PR is too big for a real code review. 🙈
From the things you listed, the biggest uncertaincy is for sure the positioning of menus, since there are basically no tests for that. The rest is (hopefully) more or less well covered. So it would be good if the Tests were green. Regarding the positioning, I couldn't find any issue however 🤷
I nevertheless would like to wait for @cguiot-OP feedback on the PR.
I did find one bug, which is however not a blocker for me and can be tackled separately: Once you save a form configuration, you cannot drag and drop any more. You have to do a hard reload, then it works again.
| // TODO: port tooltips | ||
| // jQuery(event.el).tooltip({ | ||
| // content: this.tooltipContentString(event.event.extendedProps.entry as TimeEntryResource, schema), | ||
| // items: '.fc-event', | ||
| // close() { | ||
| // document.querySelectorAll('.ui-helper-hidden-accessible').forEach(element => element.remove()); | ||
| // }, | ||
| // track: true, | ||
| // }); | ||
| } | ||
|
|
||
| private removeTooltip(event:CalendarViewEvent):void { | ||
| const target = jQuery(event.el); | ||
|
|
||
| if (target.tooltip('instance')) { | ||
| jQuery(event.el).tooltip('disable'); | ||
| } | ||
| // TODO: port tooltips |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about these Todos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find how to show these tooltips.
I guess this could be tackled in a follow up.
| public get EVENT() { | ||
| return 'contextmenu.cardView.rightclick'; | ||
| public get EVENT():EventType { | ||
| return 'contextmenu'; // N.B.: contextmenu is not supported by Safari on iOS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does that comment mean? Is right click not supported on iOS Safari any more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HDinger Good question. rightclick is a jQuery only event AFAICT (I actually can't find any docs for it though) - perhaps @oliverguenther knows?. contextmenu is a new-ish event that fires once the click is processed, but as stated isn't supported by iOS Safari. https://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event
In addition to right-clicking, it also is fired when users open the context menu via keyboard, which may mean (ContextMenuKeyboardHandler is redundant?)
The lack of iOS Safari support may be a moot point - as I'm not sure if we need context menus on mobile anyway?
That does remind me that we should also do smoke testing on mobile. @cguiot-OP do we support context menus on mobile?
This PR includes commits from #20846. Please review/merge that first.Ticket
https://community.openproject.org/wp/65618
What are you trying to accomplish?
Remove jQuery calls from most parts of the core OpenProject codebase.
While jQuery was revolutionary when it was first released, it is an obsolete technology. Nearly all of jQuery's functionality can be replaced by newer Web APIs.
This PR removes jQuery dependencies from the OpenProject core codebase, replacing jQuery APIs with native DOM APIs and modern JavaScript/TypeScript alternatives. The changes include migrating event listeners, DOM manipulation, element selection, CSS manipulation, and AJAX operations away from jQuery while maintaining the same functionality.
Key changes:
document.querySelector/querySelectorAll.on(),.off()) with nativeaddEventListener/removeEventListener:delegatefrontend/src/app/shared/helpers/event-helpers.ts)JQuery.TriggeredEventto nativeEvent,MouseEvent,KeyboardEvent.csp_onclickRails helper..js.erbfiles that no longer appear to be in use.Screenshots
There should not be any significant visual changes.
What approach did you choose and why?
Work on this PR was started a couple months back and then put on hold. Originally I had planned to migrate the codebase to Luxon (#19379) first), but I realised that jQuery should probably take priority and so decoupled the two pieces of work.
Migrating from jQuery was mostly grunt work in the first phase - search and replace (with various weird Regexes) based on my own knowledge. However this PR has had many passes that have involved both me checking my work and Copilot reviewing. Copilot coding agent has also worked on some fixes (e.g. #20909, #20908, #20906). Fixing the final 50 or so failing tests took the most time and involved running feature specs headful (e.g.
OPENPROJECT_TESTING_AUTO_DEVTOOLS=true OPENPROJECT_TESTING_NO_HEADLESS=1 bundle exec rspec ./spec/features/work_packages/export_spec.rb) and manual debugging.Scope
Follow up PRs
These PRs can be reviewed separately, but are based on this PR and should be merged after it. Work was split out for ease of review (this PR is already huge) and/or not being strictly in scope.
What this PR does not do
This work package will not remove the jQuery dependency or global. This is to give us more time to test plugins and because there are still some modules with a jQuery requirement: BIM, Reporting (used in a few places), and Backlogs (used extensively).
There are already related (WIP) Pull Requests for Reporting and Backlogs:
I didn't fix (many) eslint issues on existing code (Copilot worked on follow up PR that #20939 fixes some issues) or perform any Angular migrations (these should be done separately).
Other things this PR does
getMetaElement,getMetaValueandgetMetaContenthelper functions.Merge checklist
Note for reviewers: this is a huge PR and there are a lot of comments. I've marked questions and things I was unsure about with "Check". These could do with a human review!